feat: adding piped process to replace PTY when needed #8797
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10f96c1acc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb9726a3c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
# Conflicts: # codex-rs/core/src/unified_exec/process_manager.rs
| @@ -0,0 +1,200 @@ | |||
| use std::collections::HashMap; | |||
There was a problem hiding this comment.
would be nice to rebase shell_command on top of this
There was a problem hiding this comment.
Yep! In the end I think it will allow us to keep only unified_exec
There was a problem hiding this comment.
there is lots of logic in /Users/pakrym/code/codex/codex-rs/core/src/exec.rs that I'm afraid we'll rediscover again with a separate implementation
codex-rs/utils/pty/src/pipe.rs
Outdated
|
|
||
| #[cfg(unix)] | ||
| fn kill_process(pid: u32) -> io::Result<()> { | ||
| let result = unsafe { libc::kill(pid as libc::pid_t, libc::SIGKILL) }; |
There was a problem hiding this comment.
we have an entire thing to kill trees of child processes in shell_command
pakrym-oai
left a comment
pakrym-oai
left a comment
There was a problem hiding this comment.
Instead of starting with a completely new pipe implementation can we reuse/extend/share the one we have for shell_command?
# Conflicts: # codex-rs/core/src/tools/spec.rs
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 556137528e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let reader_handle = tokio::spawn(async move { | ||
| if let Some(handle) = stdout_handle { | ||
| let _ = handle.await; | ||
| } | ||
| if let Some(handle) = stderr_handle { |
There was a problem hiding this comment.
The stdout/stderr reader tasks spawned here are only awaited by reader_handle, but ProcessHandle::terminate (and Drop) only aborts that one handle; the actual stdout/stderr tasks are not stored anywhere. If a child process is terminated while a grandchild keeps stdout/stderr open, those reader tasks will keep running indefinitely, leaking tasks and continuing to read from the pipes. Consider storing those JoinHandles (or wiring a cancellation token/timeout) so terminate() can abort the actual reader tasks.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
codex/codex-rs/core/src/tools/spec.rs
Lines 217 to 221 in 43f33a5
Update exec_command description to match pipe default
The tool spec still says exec_command “Runs a command in a PTY”, but this commit changes the default to pipe mode unless tty=true. Clients (including the model) often rely on the top‑level description rather than per‑field docs, so they may omit tty and end up running commands that require a TTY (e.g., interactive prompts, isatty‑gated behavior) in non‑TTY mode, causing hangs or different output. Consider updating the description to reflect the new default and the tty flag.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
No description provided.